[ci]: gate the public API surface and the published package shape - #1471
sf-tyler-jeong wants to merge 17 commits into
Conversation
The published package exposes 202 subpath entries, and what each one declares is only visible in the generated .d.ts tree — which the build throws away. So a change like dropping a context action or turning Promise<void> into void leaves no trace in the diff: tsc stays green because the library is internally consistent, and the break surfaces in a customer's build instead of ours. `yarn api:snapshot` walks the entry points in rollup.module-exports.mjs, follows re-exports and inline import types through dist/types, and writes the reachable declarations to api/surface.d.ts. Internal modules that no entry point reaches are left out — 294 of 546 files survive the filter. The file is generated, never edited. Regenerating it twice over one build gives a byte-identical result.
Regenerates the snapshot after the build and compares it with the committed copy. A mismatch means the public surface moved without anyone recording it, so the step prints the diff and fails; an intended change passes once api/surface.d.ts is regenerated and committed alongside the code. The job name is unchanged, so the existing required check covers this without touching the ruleset, and `yarn build` already runs here — the step adds no build time. It also asserts the snapshot is tracked. Left untracked, `git diff` reports no change and the gate would pass silently forever.
The snapshot is only as good as the walk that builds it. If following `export *`, a directory index, or an inline `import()` type ever regresses, the file shrinks, the diff looks empty, and the gate reports a clean surface while saying nothing — the failure mode with no symptom. Splits the walk out of the CLI so it can be driven against a fixture tree and covers it with fourteen cases: entry mapping, specifier resolution, star and named re-exports, inline import types, cycles, several entries merging, an entry whose declaration was never emitted, and the snapshot's format and determinism. Verified by breaking the walk — dropping inline import tracking fails one case, dropping index resolution fails two. vitest looked only under src/, so the include now reaches scripts/ as well. The generated snapshot is byte-identical to the one already committed. Two smaller repairs: the workflow now says why a missing api/surface.d.ts matters instead of leaving a bare pathspec error, and the script warns when src/ is newer than dist/types, since a stale tree snapshots cleanly and the mismatch would only appear in CI.
The script decided whether to run by comparing process.argv[1] with import.meta.url, so that it could also be imported by the spec. Node resolves symlinks for the module URL but leaves argv[1] as given, so reaching the repo through a symlinked path made the two differ and main() never ran. The script then exited 0 with no output, the snapshot was never regenerated, `git diff` came back empty, and the gate reported an unchanged surface. No error, no warning. On macOS /tmp is a symlink to /private/tmp, which is how the throwaway worktree used while building this reached the repo. Splitting the CLI into its own file removes the decision rather than correcting it — importing the module now does nothing on its own, so there is no condition left to get wrong. Verified through both a symlinked and a direct path; the generated file is unchanged.
…pshot Several public paths map to the same source file, so the snapshot was byte-identical after one of them was deleted: the declaration stayed reachable through its sibling. Dropping 'Channel/components/MessageInput' left the gate green while '@sendbird/uikit-react/Channel/components/MessageInput' started failing with ERR_PACKAGE_PATH_NOT_EXPORTED for anyone importing it. The walk only consumed Object.values(moduleExports), discarding the path names entirely. Emit them as a header ahead of the declarations, sorted by public path so the block does not move with declaration order. Covered by five cases that go through renderSnapshot rather than renderEntryPoints: asserting the pure function leaves them all passing when the header is unwired from the written file.
`lame.all` is the only entry whose source is plain JavaScript, so the `.replace(/\.tsx?$/, '.d.ts')` that derives the declaration path silently left it alone. The published manifest named types/_externals/lamejs/lame.all.js under both `exports` and `typesVersions`, and tsc --emitDeclarationOnly never writes that file: publint reports it as an error and the directory is absent from the tarball. Omit the `types` condition when the source emits no declaration. Against the manifest published as 3.19.0 the only differences are the dropped key and its typesVersions entry; the other 203 export entries are unchanged. attw moves the subpath from NoResolution to UntypedResolution, so consumers go from finding nothing to finding the JavaScript without types. Extract the map into its own module so the shape can be asserted without running the build.
movePackageJSON() was called without await, so its write raced the synchronous work that followed it. execSync blocks the loop until it returns, which means a failure in buildTypeDefinitions() ends the process before the continuation ever runs and dist/package.json is left missing. Observed while running the script with tsc off PATH: exit 127, no manifest. Sequence the steps and report a failure explicitly rather than as an unhandled rejection.
The surface snapshot proves the declarations did not change. It says nothing about whether a consumer can reach them: it walks dist/types from the entry sources, so an exports target that resolves nowhere is absent from the snapshot instead of visible in it. Add publint and attw over dist. Known-failing categories are excluded rather than fixed here, so the gate reports regressions only: - false-esm: `dist/package.json` declares "type": "module" while `dist/cjs` declares commonjs, so a node16/nodenext consumer emitting CommonJS gets TS1479 on every entry. bundler, legacy node and ESM consumers all typecheck. Worth its own change: correcting it means emitting a second set of declarations for the require condition across all 204 entries. - internal-resolution-error: relative specifiers inside the declaration files. A consumer with skipLibCheck disabled reports none of them, so this is not reachable from outside the package. Both entries excluded by path are untyped by nature: the stylesheet and lame.all.
c661f806 ordered the steps so dist/package.json is written before anything that can throw, but nothing held that ordering: the suite covers the export map alone and stays green when the await is dropped and tsc fails. Run the real script in a sandbox rather than against dist/, which the surface snapshot and package:check both read. A temporary tree gets the three scripts, a single-entry module map and empty dist directories, and a stub tsc ahead of PATH makes the declaration step fail the same way on any machine. Two assertions, each covering a different way the fix can rot: dropping the await loses the manifest, and swallowing the rejection loses the exit code. Neither catches the other, since an unhandled rejection also exits non-zero.
Nothing read the TypeScript this branch added under scripts/. eslint is scoped to 'src/**/*.ts*', tsconfig.json includes only src and vitest-setup.ts, and vitest strips types without checking them: a `const x: number` assigned a string passed typecheck, lint and test alike. A separate project rather than widening tsconfig.json, because the two options that touch the shared config both fail. Adding "scripts" to include alone leaves the imports from .js and .mjs typed as any, since noImplicitAny is off, so the gate reports nothing. Adding allowJs there reaches the build as well — tsconfig.build.json overrides include but inherits compilerOptions — and the declaration emit picks up five mock files under src that its exclude list does not cover, putting them in the published package. Scoped this way the emit is untouched at 546 declarations, and a type error planted in each of the three specs now fails yarn typecheck. The spec change is a cast: package_exports.js assigns its export entries through a computed key, which leaves Object.values inferring string[].
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
001d7b3 |
2026-09-18 03:52 UTC | 0 | 0 | 0 |
25825b3 |
2026-09-23 00:59 UTC | 0 | 0 | 0 |
21d535c |
2026-09-23 02:29 UTC | +1 | -1 | 0 |
43e73fe |
2026-09-23 05:40 UTC | +1 | 0 | +1 |
d47b0d4 |
2026-09-23 07:25 UTC | +1 | 0 | +1 |
b4f3df6 < |
2026-09-23 08:26 UTC | 0 | 0 | 0 |
Last scanned: b4f3df6 · 2026-09-23 08:26 UTC
✅ Deploy Preview for sendbird-uikit-react ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
| Commit | Scanned at | New | Resolved | Net |
|---|---|---|---|---|
001d7b3 |
2026-09-18 03:52 UTC | 0 | 0 | 0 |
25825b3 |
2026-09-23 00:59 UTC | 0 | 0 | 0 |
21d535c |
2026-09-23 02:29 UTC | 0 | 0 | 0 |
43e73fe |
2026-09-23 05:40 UTC | 0 | 0 | 0 |
d47b0d4 |
2026-09-23 07:24 UTC | 0 | 0 | 0 |
b4f3df6 < |
2026-09-23 08:26 UTC | 0 | 0 | 0 |
Last scanned: b4f3df6 · 2026-09-23 08:26 UTC
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
| return ( | ||
| header + | ||
| declarations | ||
| .map((file) => `// ===== ${relative(typesDir, file)} =====\n${readFileSync(file, 'utf-8')}`) |
There was a problem hiding this comment.
파일을 통째로 담으면 공개 표면과 무관한 변경까지 스냅샷이 흔들립니다. export 하지 않은 내부 타입 수정이나 import 경로 변경 같은 것들인데, 커밋된 7,228줄 중 import 문만 814줄입니다. 내부 파일 하나를 옮겨도 CI 가 "공개 API 표면이 바뀌었다"로 실패하게 됩니다.
파일 단위보다 export 된 심볼 단위로 비교하는 쪽이 좋겠습니다.
There was a problem hiding this comment.
진단이 맞았습니다. 수치도 그대로 확인했습니다 — 7,228줄 중 import가 814줄이고, 내부 파일 하나를 옮기면 경로만 바뀌는데 CI가 "공개 API 표면이 바뀌었다"로 떨어집니다.
내부 선언은 심볼 단위로 바꿨습니다. 파일 경로는 더 이상 키에 들어가지 않고, 참조도 대상 파일이 아니라 그 심볼을 가리킵니다. 그래서 어떤 파일에 무관한 export가 하나 늘어도 그 파일을 참조하던 곳이 움직이지 않습니다. 실제로 src/를 고치고 빌드해서 다시 측정한 값입니다.
| 바꾼 것 | 스냅샷 |
|---|---|
| 내부 파일 이름 변경 | 0줄 |
| 내부 시그니처 한 줄 변경 | 2줄 (그 줄만) |
| 공개 export 추가 | 1줄 |
| 공개 subpath가 이름 하나를 잃음 | 2줄, 전부 - |
다만 진입점은 파일 단위로 남겼습니다. 어떤 subpath가 어떤 이름을 내보내는지가 그 자체로 계약이기 때문입니다. 위 마지막 행이 그 경우인데, "이 심볼이 트리 어딘가에 존재한다"만 기록하는 스냅샷에는 비교할 대상이 없습니다 — 선언은 그대로 있고 그 subpath가 더 이상 내보내지 않을 뿐이라서요. 여기에 더해 같은 이름을 서로 다른 선언이 쓰는 경우가 21개 있어, 이름만으로는 어느 쪽이 바뀐 건지도 가려지지 않습니다.
이 선택에는 대가가 있고, 그건 남겨 뒀습니다. 선언을 공개 모듈 밖으로 옮기면 re-export로 계약을 유지하더라도 그 블록이 다시 찍힙니다. subpath가 이름을 잃은 것을 볼 수 있게 해주는 성질과 같은 것이라 둘을 떼어낼 수가 없었습니다. 리뷰어가 "이 이동이 맞는지" 한 번 보게 되는 쪽이 낫다고 판단했습니다.
생성기에는 테스트를 붙였습니다. 단위 테스트 112개와, 별도로 yarn api:conformance 48개입니다. 뒤쪽은 실제 컴파일러로 선언을 만들고, 소스를 바꾼 뒤 소비자 코드를 타입체크해서 소비자가 깨지는 것과 스냅샷이 움직이는 것이 서로 맞는지를 단언합니다. 놓치는 쪽과 헛짚는 쪽을 한 번에 보려고 그렇게 했습니다. 스냅샷은 어느 환경에서 돌려도 같은 바이트가 나옵니다.
감사합니다.
Recording each declaration file verbatim under its path meant the snapshot moved whenever a path did. Renaming one internal helper rewrote seven lines — a path header and six import specifiers — with no declaration body touched, and the build failed with "Public API surface changed". Moving that file across directories churned 46. A gate that fails on changes it has no opinion about is one reviewers learn to skim, and the worth of this file is that an empty diff settles the question. Internal declarations are now emitted one block per symbol, keyed by the symbol; several declarations sharing a name in one file are one symbol, as the const-plus-type pair behind an enum is. Entry points keep whole- file blocks, keyed by the public subpath: which symbols a public module exports is itself the contract, and discarding that is what makes a purely symbol-keyed snapshot unsafe. Move a type out of GroupChannel/components/TypingIndicator without re-exporting it and the subpath stops exporting it, while a snapshot that only records "this symbol exists somewhere" reports nothing. Here it moves 10 lines. A symbol is identified by what it declares and by what that declaration refers to — imported types and types declared beside it alike — resolved to a fixed point that ends when the keys stop moving rather than after a set number of passes. A chain of aliases is as long as someone wrote it, and a difference at its far end still has to reach the name a public path exports; each pass re-renders only the symbols whose references moved — tracked as the keys a rendering actually read, not scraped back out of its text, so that a reference wearing a type-only marker or sitting inside a namespace still counts, and carried along when a second declaration reuses a lookup the first one already made — which is what keeps a five-hundred-deep chain to under a second instead of forty. Two interfaces that read identically but reach for different types are told apart, and a reference carries the same key its block does. A reference names the symbol it reaches for rather than the file that happens to hold it, so an unrelated export joining that file moves nothing. Names are followed to their declaration through aliases, through a binding that was imported and then exported on, through a namespace, and out to a package, whose specifier is kept since that is as far as this repository can see. What a module publishes a name as and what it calls that name inside itself belong to two namespaces, and each is followed on its own terms: an export name through the module's export list, its explicit re-exports and its stars; a local name through its declarations and its imports; the first handing to the second where an export list renames on the way out. One lookup answering for both is wrong in both directions — a module that publishes one local name under another's sends that other name to the wrong declaration, and a name reaches a declaration through an `import type` the module never exported, which is where the ChannelSettings entry had been picking up a type-only marker it has no claim to. `default` is an export name like the rest, reached through a rename, a re-export, or a plain `export default`, rather than by matching the text of the last. A walk after one name records the name it was after at each module it passed and not the module alone, in both namespaces: arriving at a file a second time for a different export is how a pair of barrels that hand names to each other reads, and calling that a circle abandons the walk one hop short of the declaration. Whether a name leaves type-only travels with it, because that is the difference between a consumer constructing a class and failing to. Two forms point at the module because the module is what they mean — `export *` and `import * as` — and a module is keyed by what it exports: every exported name, the symbol it resolves to, and whether it leaves type-only. A star is spread before anything written out beside it — a declaration, a local export list, an explicit re-export — so those win the way TypeScript gives them, both when a module's exports are read and when a single name is chased to its declaration. What it spreads is every exported name but `default`, which the language does not forward: take away the explicit default re-export standing beside a star and the module has stopped offering a default. A namespace a module imports and then exports means the module it was taken from, whether it goes out under that name or another. `export * as` publishes one name rather than spreading a module, and is recorded that way; every star that leaves for a package keeps the package it left for, including when a barrel passes several of them on at once. Modules that re-export each other as namespaces would otherwise walk in a circle, so each walk remembers where it has been — down its own path, so that two branches reaching the same module are not mistaken for a loop, and through a named re-export, which is another way into the same circle. Breaking the circle keeps which module it turned back to, since turning back to one module is not the same contract as turning back to another — by where it sits on the path, so that modules which declare nothing of their own and only pass names through are still told apart. A result reached that way belongs to the path that found it, so it stays out of the cache rather than being handed to the next reader, whose path may differ and whose file names have nothing to do with the contract, which holds for a circle of plain stars as much as for one of namespaces. An internal barrel is otherwise free to drop a name, rename one, narrow one to a type or repoint one elsewhere while the snapshot stays still. A block carries the references that bind the names it uses, and only those. Every import is split one name per line — package imports included, their specifiers left alone since they name another package's contract — and the set is computed from every local binding a statement introduces, named, aliased, default and namespace alike. Attaching a whole statement instead would pull in names the declaration never mentions, which is enough to give one shared declaration two different bodies and split it into two blocks. Checked against real builds rather than edited declarations. Five reorganisations that leave the public surface alone produce no diff at all: rename, directory move, split behind a barrel, moving a declaration between internal modules, and moving an entry's source while its subpath stays. Five real changes still show — a changed internal signature (2 lines), a new public export (1), an entry losing a symbol (10), an entry removed (63), and a prop added to three same-named public types (3). Two lines is what a symbol whose name is its own alone costs; where a name is shared, its key carries a digest of what the symbol declares, so a one-line change moves the key along with the body and reads as twelve. The entry block draws the boundary. Moving a declaration out of a public module shows at 11 lines even when a re-export keeps the subpath exporting it, because the block records that module verbatim. That is the same property that lets the snapshot see a subpath lose a symbol, so the two come together; resolving re-exports into the block instead would expand every entry's closure into it. Nothing is dropped in the move: every declaration line of all 294 collected files is present, along with all 725 exported names, 93 @deprecated tags, and 201 of the 202 public subpaths — the one left out is vendored JavaScript, which the compiler emits no declarations for and so has no surface to record. All 730 block keys are unique, every reference either names a block, a module, or a package, and the file regenerates byte-identically in under two seconds — on any machine. Blocks are laid out by comparing their keys as strings rather than by a collation, which changes with the shell's locale: under a Turkish one the old comparator moved six lines on its own, and a gate read by `git diff --quiet` would have called that a changed public surface. Symbol-level extraction can shrink silently — a declaration the splitter fails to recognise leaves no trace, and a smaller snapshot reads as a clean surface. The spec asserts every line of every collected file survives, that keys stay unique, that a block keeps the reference binding each name it uses and none it does not, and that a reference does not move when the file behind it gains an unrelated export. It covers the shapes that are easy to drop or to over-collect: a declaration behind a doc comment, a file-level comment ahead of the imports, a module's local export list, a side-effect import with no name to attach it to, a binding that is not the first name in its import, a default and named import sharing one statement, and the const-and-type pair one name can stand for, where keeping only the declaration written last drops the other without narrowing anything the snapshot reports. Each was confirmed by reverting the fix and watching the suite go red.
The snapshot spec writes its declaration fixtures by hand, which means it can only assert what the person writing it already believed. A fixture can be a rung shallower than the shape it is standing in for, and then reverting the fix under test leaves the suite green — which happened while this generator was being reviewed. A fixture that is wrong in the same direction as the code proves nothing. `yarn api:conformance` derives both ends from tools instead. Each case is a small package of TypeScript sources: the real compiler emits the declarations, the generator snapshots them, the sources change, and the compiler then type-checks a consumer against the rebuilt package. The case passes when the consumer breaking and the snapshot moving agree — a change a consumer feels has to show, and one it does not feel must not. The second half is the half the spec was weakest on, and it is what caught a file rename turning into a reported API change. Thirty-eight cases, each one a shape that slipped past the generator at some point: a name narrowed to type-only along a re-export chain, a binding imported and then exported on under another name, an export list whose public names collide with the local ones it draws from, a namespace renamed, repointed, or published by name, a re-export leaving for a different package, a star asked to carry the default export behind it, a default import landing on a module that only re-exports one, a chain that passes back through a module it has already been to for a different name, one name standing for both a const and a type, two declarations that read alike but reach for different types, modules that re-export each other in a circle. `--deep` adds a five-hundred-link alias chain, which takes about a minute on its own. A case can also pass while standing for nothing, so the runner refuses the three ways that happens: a change naming a file the case never compiles, a change rewriting a file with the text it already had, and a build whose emit the compiler skipped — the last of which would leave the previous build in place and snapshot it a second time. Kept out of `yarn test`, which it would roughly double. This is the suite to run when `scripts/api_surface.mjs` changes, and reverting any of that file's handling of stars, cycles, aliases, namespaces or type- only exports fails it.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25825b3afe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A module written for Node's ESM resolution names its neighbour with the
extension that neighbour will have at runtime — `export { Foo } from
'./foo.js'` — and declaration emit keeps the specifier as written while
emitting the target as `foo.d.ts`. Probing for `foo.js.d.ts` and
`foo.js/index.d.ts` finds neither, so the target never joins the
collected set and the snapshot holds no record of what `Foo` is:
changing it leaves the file byte-identical while a consumer stops
compiling. A gate that reports nothing is worse than no gate, because an
empty diff is read as an answer.
The runtime extension is now substituted the way the compiler does it
before the candidates are probed, and the substituted form is tried
first: `.js` and `.jsx` stand for a `.d.ts`, `.mjs` and `.cjs` for a
`.d.mts` and a `.d.cts`. Where none of those exist the specifier is read
literally, again as the compiler reads it — `./foo.cjs` beside both a
`foo.ts` and a `foo.cjs.ts` means the second, and letting a bare `.d.ts`
stand in for the missing `.d.cts` would answer with the first. A
specifier that names a declaration file outright is taken as it stands:
`import type { Foo } from './foo.d.ts'` is what the compiler suggests
when it turns the plain import down, it type checks, and declaration
emit keeps it word for word. Nothing in this package spells an extension
out today, so the snapshot does not move — the point is that being right
about it costs nothing before something does.
Checked by resolving every arrangement of the files a specifier could
mean, 112 of them, against the compiler's own answer.
Covered by two conformance cases, where the compiler emits the
declarations, the type behind the specifier changes, and the consumer
and the snapshot have to agree, and by three tests over the resolver
itself, since `.jsx` needs a compiler flag the conformance fixtures do
not set and a declaration file is not re-emitted into an output
directory at all. Dropping the substitution fails two of the tests and
one of the cases; keeping it but letting the bare `.d.ts` stand in fails
one of each; taking the declaration extension away fails one test and
puts four arrangements back out of step with the compiler.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21d535c213
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more ways a declaration went unread. The snapshot is only as good as the set of files it reaches, and a file it never reaches is a change it can never report — the diff comes back empty, and an empty diff is read as an answer. A triple-slash `<reference path="...">` names a declaration the way an import specifier does, and a public signature can rest on a global it brings in. Only `from` and `import()` were being scanned, so the referenced file stayed outside the collected set and whatever it declared could change unremarked. Reference paths are followed now alongside the specifiers, and it is the compiler's own directive parser that reads them: matching the shape by hand went wrong in six of eighteen spellings, once by missing a file the compiler reads — an attribute name is not case sensitive — and five times by collecting one it does not, among them a directive with no closing slash and a directive quoted inside a string. A default export carries the name of whatever it declares, so an interface as well as a function or a class — those three and no others, since an enum or a namespace cannot be default-exported at all. Reading `export default interface Shape` as anonymous gives two modules that default-export different shapes the same export signature, and so the same module key: point an entry at the other one and nothing moves while a consumer stops compiling. This package already has such a module, under `modules/App`, though nothing reaches it by module key today. Neither changes the snapshot; reading the directives costs the run about a third of a second, spent once. Covered by two conformance cases, where the compiler emits the declarations, the type behind the reference changes, and the consumer and the snapshot have to agree, and by three tests over the walk itself, one of them holding the directive grammar to what the compiler accepts. Reverting either fix fails one case and one test; the default one has to swap which module an entry points at rather than edit a type in place, since editing one rewrites its block and would pass whether the module key were right or not.
The test that carries a difference down a 520-link alias chain takes about 1.9s on its own and 5.5s when the whole suite is running beside it, which is over vitest's 5s default. It passes when run alone and fails when run with everything else, which is the shape of a test that fails in CI and not on the machine of whoever is asked to look at it. The length is the point — the chain has to be longer than any fixed number of passes the generator could make — so the budget is what gives rather than the fixture. Thirty seconds leaves room for a loaded runner without letting a genuine hang sit there.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43e73fe8e9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
One statement can bind several names — `export declare const foo = "", bar = 1;` is what declaration emit gives back for a source that wrote them together — and only the first was being recorded. The second belonged to no symbol, so a reference to it resolved to nothing and every module that declared a name by that spelling folded to the same key. Point a public entry at a different one of them and the snapshot holds still while a consumer stops compiling. `const enum E` went the same way for a different reason: the reader took `const` for the whole of the declaration and called the thing `enum`. Every name a statement binds is recorded now, against the statement they share. Which names a declaration refers to is then read from the shape of the block rather than from its text, because an identifier sitting where a name is being declared refers to nothing, while the same identifier in a type position refers to something. Both halves of that matter and they pull opposite ways: a value can be typed by a type of the same name declared beside it, and losing that reference makes two unlike modules read alike, while counting the name written beside it makes two names depend on each other, and then an unrelated module leaving the tree moves both of their keys. The exception is a property keyed by a name rather than labelled with one — `[KEY]: string` is a reference to the `KEY` declared above it, and that subtree is read rather than skipped with the rest of the name. The names are read with the compiler's parser rather than by splitting on commas, because the commas that matter are not the only ones on the line: `Map<string, number>` and `(a: string, b: number) => void` each carry one that binds nothing. Only statements that open with `const`, `let` or `var` are parsed for their names — everything else takes its name from the declaration line, as before — so the run is no slower. One block moves: a `_default` whose property happens to carry the name of a declaration in the same file was depending on it, though the property's type is written out in full and never names it. Changing that declaration still moves the block, through its body. Nothing else here writes two names into one statement or declares a `const enum`, so the rest of the snapshot stands. Covered by six conformance cases that each hold a public entry still while the modules behind it move, and by eight tests over the reader, one of them holding the commas inside a type. Recording only the first name again fails two cases and three tests; reading the const enum by the line fails one of each; leaving out every name written beside the one being keyed fails one of each; taking the declaration sites for references fails one case and two tests; and skipping a computed property name with the rest of the name fails two cases and one test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d47b0d4174
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Two more ways the walk and the module signature lost track of something a
consumer can feel.
A side-effect import names a declaration as surely as one with a `from`
clause. `import './augment';` is how a module brings in a global or an
augmentation, and a public signature can rest on what it declares, but
only specifiers with a `from` or an `import()` were being scanned, so the
file stayed outside the collected set and whatever it declared could
change unremarked. The compiler's preprocessor already reads that file
for its reference directives; its list of imported files is read along
with them now, and the relative ones are followed.
A module that default-exports something without naming it —
`export default class { ... }` — had that recorded as the word
`anonymous`, so two such modules had the same export signature and the
same module key. Point a public alias at the other one and nothing moves
while a consumer's type changes underneath. The declaration is in the
snapshot already, under a key derived from what it says; the signature
carries that key now instead of a word that means nothing.
Neither changes the snapshot: nothing here imports a declaration for its
side effect — all 114 bare relative imports in the collected declarations
are stylesheets — and nothing default-exports without a name. Covered by
a conformance case each, both of which swap which of two modules a public
entry reaches for, and by a test each over the walk and the snapshot.
Reverting either fails one case and one test.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b4f3df6a07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Summary
Public API surface changes are caught by a person reading a diff. Props, exports, types and the shape a context or hook returns are all contracts a customer builds against, and once one of them slips out it cannot be taken back.
This checks a machine-written snapshot of that surface into the repository and has CI regenerate and compare it on every build. A surface that moves fails the build. An intended change is committed together with the snapshot, so the diff says exactly what moved.
The same idea is applied to the shape of the published package, with
publintand@arethetypeswrong/cli. Turning that on surfaced two real defects, fixed here.Changes
The surface snapshot —
api/surface.d.tsStarting from the entry points in
rollup.module-exports.mjs, the generator collects every declaration reachable from them throughdist/types— every relative import a declaration carries, including the ones written for their side effect alone, and every<reference path>directive. Package specifiers are not followed: those types belong to their own packages.The file is keyed by what it records, not by where it came from. A path in a key would make the gate fail on changes it has no opinion about — renaming one internal helper rewrote seven lines, and moving it across directories rewrote forty-six, with no declaration body touched. A gate that fails on those is one reviewers learn to skim.
So the two halves are keyed differently, because they are different contracts:
Names are followed to their declaration through aliases, through a binding imported and then exported on, through a namespace, and out to a package, whose specifier is kept since that is as far as this repository can see. Whether a name leaves type-only travels with it, because that is the difference between a consumer constructing a class and failing to.
Blocks are laid out by comparing their keys as strings rather than by a collation, which changes with the shell's locale. The file regenerates byte-identically on any machine, in under two seconds.
The snapshot opens with the list of entry point names. Two entry points that share a source are recorded under one key carrying both names, so dropping one of a pair already shows there —
Channel/components/MessageInputandChannel/components/MessageInputWrapperpoint at the same source today, and removing either rewrites that key. What the list catches on its own is an entry point with no declarations to record: droplame.alland the one line that moves is its name.CI regenerates after
yarn buildand compares withgit diff --quiet. It first refuses to run ifapi/surface.d.tsis untracked, since a deleted file makes the comparison pass silently.Measured against real
srcedits and rebuilds, not hand-written fixtures:The boundary has a cost, and it is deliberate. Moving a declaration out of a public module shows even when a re-export keeps the subpath exporting it, because the entry block records that module verbatim. That is the same property that lets the snapshot see a subpath lose a name, so the two come together; resolving re-exports into the block instead would expand every entry's closure into it.
Why not an existing tool
Both candidates were installed and run against this package before the generator was written.
@microsoft/api-extractordoes this workflow canonically but models one entry point per run: 662ms each here, so about 134 seconds for 202 entry points, plus a config and a committed report for each. Its report also omits types that are referenced but not exported from that entry point — nineteenae-forgotten-exportwarnings in the root report alone — so a change to one of them would not show.tsnapimatches the per-subpath model and is fast, but expects a bundled.d.tsper entry. Against a declaration tree it leaves nineteen of the 203 entries it produces as unresolved relative re-exports, the root.among them.Neither is a drop-in for a package that ships a declaration tree with 202 subpaths.
The published package shape —
yarn package:checkpublintreads the manifest andattwresolves every entry point the way a consumer would. Both were run by hand first, and what they turned up is fixed below.Two rule classes the package already triggered before this change —
false-esmandinternal-resolution-error— are excluded, so the gate reports regressions instead of requiring the existing baseline to be cleared first. The two entry points excluded by path are untyped by nature: the stylesheet andlame.all.Two defects it found
The manifest pointed at a declaration file the package never shipped.
lame.allis the only entry point whose source is plain JavaScript, so the.replace(/\.tsx?$/, '.d.ts')that derives the declaration path matched nothing and left the.jsextension in place. BothexportsandtypesVersionsnamedtypes/_externals/lamejs/lame.all.js, whichtsc --emitDeclarationOnlynever writes — the directory is not in the tarball.publintreports it as an error. Thetypescondition is now omitted when the source emits no declaration. Against the manifest published as 3.19.0, the only differences are that key and itstypesVersionsentry; the other 203 export entries are unchanged.A failed build left the manifest unwritten.
post_build.jscalledmovePackageJSON()without awaiting it, so its write raced the synchronous work behind it.execSyncholds the loop, so a failure in the declaration build ended the process before the continuation ran anddist/package.jsonwas left missing. The steps are sequenced and a failure is now reported explicitly instead of as an unhandled rejection.Type checking for
scripts/Nothing read the TypeScript this branch adds under
scripts/: eslint is scoped tosrc/**/*.ts*,tsconfig.jsonincludes onlysrc, and vitest strips types without checking them. A separate project rather than widening the shared config: settingallowJsthere would reach the build too, sincetsconfig.build.jsonoverridesincludebut inheritscompilerOptions. Measured, that alternative grows the declaration emit from 546 to 551 and puts five mock files into the package, so it was rejected. Scoped this way the emit is untouched.Backward compatibility
No public API changes. Not a line of
src/is touched, and props, exports, types, context return shapes, CSS class names and StringSet keys are all unchanged.The one manifest difference is the
typespointer removed fromlame.all, and it changes nothing for a consumer. Measured against a package with the old shape and one with the new, TypeScript reports the sameTS7016and resolves to the same JavaScript file underbundler,node16andnode10alike — it falls through atypescondition that resolves to nothing. A control with a real.d.tscompiles clean, so the comparison does distinguish the cases. What changes is that the manifest no longer names a file that is not in the package, which is whatpublintandattwread.Testing
yarn lint,yarn typecheck,yarn build,yarn test,yarn api:snapshotandyarn package:checkall pass. Every commit on the branch was also checked out on its own and passes lint,tsc, the generator tests and the conformance suite, and reproduces the snapshot it carriesui/Togglefromrollup.module-exports.mjsand regenerating drops 63 lines from the snapshot, every one of them a removal, including its entry point line — andgit diff --quietfailsyarn api:conformance— 50 casesA hand-written fixture can only assert what the person writing it already believed, and one that is wrong in the same direction as the code proves nothing. This suite derives both ends from tools instead: the real compiler emits the declarations, the generator snapshots them, the sources change, and the compiler then type-checks a consumer against the rebuilt package. A case passes when the consumer breaking and the snapshot moving agree — a change a consumer feels has to show, and one it does not feel must not.
Each of the 50 is a shape that slipped past the generator at some point: a name narrowed to type-only along a re-export chain, a binding imported and then exported on under another name, a namespace renamed or repointed, a re-export leaving for a different package, a star asked to carry a default export, a chain passing back through a module it has already been to for a different name, two declarations that read alike but reach for different types, modules that re-export each other in a circle.
--deepadds a five-hundred-link alias chain. It is kept out ofyarn test, which it would roughly double.The generator's own tests — 114
Symbol-level extraction can shrink silently: a declaration the splitter fails to recognise leaves no trace, and a smaller snapshot reads as a clean surface. The spec asserts every line of every collected file survives, that keys stay unique, that a block keeps the reference binding each name it uses and none it does not, and that a reference does not move when the file behind it gains an unrelated export. Each guard was confirmed by reverting it and watching the suite go red.
Standing invariants on the committed file: every declaration line of all 294 collected files is present, along with all 725 exported names, 93
@deprecatedtags and 201 of the 202 public subpaths — the one left out is vendored JavaScript, which the compiler emits no declarations for. All 730 block keys are unique, and every reference either names a block, a module, or a package.Reverted to count the failures
Each fix was reverted and the failures counted, then restored and confirmed by hash:
renderSnapshot→ 1 failureawaitout ofpost_build.js→ 1 failurenullreturn out ofdeclarationPath→ 4 failuresconst a = 1, b = 2;recorded → 2 cases and 3 testsChangelogs